tests/test_global_config.py (2)

122-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a barrier so the isolation test actually interleaves.

Each worker sets and immediately reads its key, so the tasks can (and often will) run to completion sequentially and the test would pass even if config were process-global. A threading.Barrier(4) between the write and the read forces genuine overlap.

💚 Suggested change

+        barrier = threading.Barrier(4)
+
         def worker(thread_id: int, key: str):
             shade.api_key = key
-            # Simulate work
+            barrier.wait(timeout=5)
             gateway = Gateway()
             resolved_key = gateway.api_key
             results[thread_id] = resolved_key

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_global_config.py` around lines 122 - 140, Update
test_concurrent_threads_do_not_bleed to create a threading.Barrier for all four
workers, have each worker wait at the barrier after assigning shade.api_key and
before reading Gateway().api_key, and pass the barrier into each submitted
worker so the test forces concurrent interleaving.

---

96-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Missing coverage for the instance environment override.

test_instance_api_key_beats_global and test_instance_api_base_beats_global cover key/base, but there is no test for Gateway(environment="production") while shade.environment == "sandbox" — which is exactly the path that is currently broken (see src/shade/gateway.py Lines 63-81). Adding it would have caught the regression. Want me to draft it?

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_global_config.py` around lines 96 - 117, The
TestInstanceOverridesBeatsGlobalConfig test class lacks coverage for
instance-level environment precedence. Add a test alongside
test_instance_api_key_beats_global and test_instance_api_base_beats_global that
sets shade.environment to "sandbox", constructs Gateway with
environment="production", invokes the existing request flow, and asserts the
request uses the production environment rather than the global sandbox value.

src/shade/gateway.py (2)

101-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Dead _base_url property.

Nothing in Gateway reads it — requests resolve their URL inside the HTTP clients/get_config. It's the only place the instance environment is honored, which makes the gap above easy to miss. Either delete it or make the request path use it.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/shade/gateway.py` around lines 101 - 107, The unused Gateway._base_url
property should not remain disconnected from request URL resolution. Remove
_base_url and its environment-based fallback, or update the Gateway request path
and HTTP client configuration to consistently use it; preserve the intended
precedence of _api_base, _config.api_base, and environment.base_url.

---

8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

get_config is imported but unused, and the eager validation is now redundant.

Nothing in Gateway calls get_config; the import is dead. The validate_client_settings(...) block also duplicates what SyncHTTPClient.__init__, AsyncHTTPClient.__init__ and get_config already do — three places to keep in sync. Keeping the constructor-time check for fail-fast ergonomics is fine, but drop the unused import.

Also applies to: 57-61

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/shade/gateway.py` at line 8, Remove the unused get_config import from the
gateway module and eliminate the redundant eager validate_client_settings block,
while preserving constructor-time validation if it is intentionally retained for
fail-fast behavior. Update the relevant Gateway initialization flow without
changing the existing client constructors or configuration behavior.

src/shade/client.py (1)

25-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Base URL trimming is inconsistent with get_config.

config.api_base is returned untrimmed here while .rstrip("/") binds only to the environment branch; get_config trims both. Reuse the resolver to avoid two sources of truth.

♻️ Suggested change

     `@property`
     def base_url(self) -> str:
         if self._base_url:
             return self._base_url
-        return config.api_base or config.environment.base_url.rstrip("/")
+        return (config.api_base or config.environment.base_url).rstrip("/")

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/shade/client.py` around lines 25 - 29, Update the base_url property to
reuse the existing get_config resolver instead of independently selecting
config.api_base or config.environment.base_url. Preserve the _base_url override
while ensuring the resolved configured URL follows get_config’s trimming
behavior.

src/shade/config.py (1)

47-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Collapse the six identical property/setter pairs into a descriptor.

All six accessors share the exact same body modulo field name; a small descriptor removes ~80 lines and guarantees the thread-local/global logic stays consistent when fields are added.

♻️ Sketch

class _ConfigField:
    def __init__(self, name, default, parse=None):
        self._attr = name
        self._global = f"_global_{name}"
        self._default = default
        self._parse = parse

    def __get__(self, obj, owner=None):
        if obj is None:
            return self
        if hasattr(obj._local, self._attr):
            return getattr(obj._local, self._attr)
        with obj._lock:
            return getattr(obj, self._global)

    def __set__(self, obj, value):
        if self._parse is not None:
            value = self._parse(obj, value)
        setattr(obj._local, self._attr, value)
        if threading.current_thread() is threading.main_thread():
            with obj._lock:
                setattr(obj, self._global, value)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/shade/config.py` around lines 47 - 130, Replace the six repetitive
accessor pairs in the configuration class with a reusable _ConfigField
descriptor implementing the shared thread-local/global get and set behavior.
Declare descriptors for api_key, api_base, environment, timeout, max_retries,
and debug, passing parse_environment only for environment and preserving each
field’s existing defaults and types. Remove the corresponding property and
setter methods while keeping main-thread global synchronization unchanged.

src/shade/http.py (1)

118-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove the unused helper.

_retry_with_backoff has no callers; both HTTP clients implement their own retry loops, so delete the helper unless the sync flow is rewritten to use it.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/shade/http.py` around lines 118 - 133, Remove the unused
_retry_with_backoff helper, including its retry loop and related implementation,
since no callers use it. Leave the existing retry logic in both HTTP clients
unchanged; do not rewrite the sync flow to adopt the helper.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes